Closed Bug 1356546 Opened 7 years ago Closed 7 years ago

Avoid repeated structured clones when transporting add-on messages

Categories

(WebExtensions :: General, defect, P1)

Product:
WebExtensions
Component:
General
Version:
52 Branch
Type:
defect

Tracking

(Performance Impact:high, firefox55 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla55
Project Flags:
Performance Impact high
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: edmundas.ramanauskas, Assigned: kmag)

References

Details

(Whiteboard: [triaged])

Attachments

(5 files)

bug_report.zip
8.93 KB, application/zip
Details
Bug 1356546: Part 1 - Add a StructuredCloneHolder JS helper to hold opaque structured clone blobs.
59 bytes, text/x-review-board-request
billm
: review+
Details
Bug 1356546: Part 2 - Use StructuredCloneHolder as transport for MessageManager messages.
59 bytes, text/x-review-board-request
aswan
: review+
Details
Bug 1356546: Part 3 - Use StructuredCloneHolder as transport for proxied message listeners.
59 bytes, text/x-review-board-request
aswan
: review+
Details
Bug 1356546: Part 4 - Use StructuredCloneHolder as transport for proxied method return values.
59 bytes, text/x-review-board-request
aswan
: review+
Details
Attached file bug_report.zip — 
User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/57.0.2987.133 Safari/537.36

Steps to reproduce:

I'm developing an extension for firefox. in that extension we post messages between background and popup scripts. when posting a large objects it takes a long time to deliver a message. even previously sent 'lighter' are delayed. I found also a workaround for this issue: use JSON.stringify before posting the data. I'm attaching a demo extension where you can reproduce this behavior. in popup you'll see 2 buttons: one will post a regular message and second one - stringified. you'll see that there's a big difference in delivery time.
also you can check the code in my github repository:
https://github.com/edmundas-ramanauskas/firefox-bug-demo


Actual results:

there's a big difference in delivery time between regular message and stringified message


Expected results:

the difference should be negligible
I assume it's extension using the WebExtensions API. Retriage if that's not the case.
Component: Untriaged → WebExtensions: Untriaged
Product: Firefox → Toolkit
Yes, it's WebExtensions API.
can you investigate this Bob?  around telemetry info
Flags: needinfo?(bob.silverberg)
I've been considering doing something about this for a while. Basically, we just need to structured clone write the object directly from the extension compartment to an opaque buffer that we can pass around, and then structured clone read it from that buffer directly into the target extension compartment.
Status: UNCONFIRMED → NEW
Component: WebExtensions: Untriaged → WebExtensions: General
Ever confirmed: true
Summary: postMessage in firefox extension takes a lot of time → Avoid repeated structured clones when transporting add-on messages
The issue here is that you are passing a very large complex object into postMessage, and in the background the framework has to clone this object to send the message, and that takes a long time. When you stringify the object first it saves all that extra cloning, because now there is just one large string to clone, instead of thousands of objects.

This is a known issue, and you have found a workaround, which is probably your best bet for now. As Kris mentioned above, he's been considering adding some utilities to make this process faster, without a developer having to stringify an object before passing it into a postMessage call. This bug will be used to track that work.
Flags: needinfo?(bob.silverberg)
Whiteboard: [qf]
Priority: -- → P3
Whiteboard: [qf] → [qf][triaged]
Whiteboard: [qf][triaged] → [qf:investigate:p1][triaged]
Kris, do you have a good sense of how big of an issue this bug is?  And do you have a sense of the degree of difficulty of fixing it?
Flags: needinfo?(kmaglione+bmo)
I'm not sure how big of an issue it is in practice. It would depend on how much message passing extensions are doing, and how much data they're passing in each message. But bsilverberg tested with the absurdly large example message above and says that there's a very noticeable delay when sending it.

As for how difficult it is to fix, probably not very. It shouldn't take me more than an hour or two, at my best guess, if I go with one of the simpler solutions.
Flags: needinfo?(kmaglione+bmo)
Given bug 1362792, I'd say this is probably a major issue.
Blocks: 1362792
Ugh, yeah that certainly increases the importance of this...
Whiteboard: [qf:investigate:p1][triaged] → [qf:p1][triaged]
Assignee: nobody → kmaglione+bmo
Priority: P3 → P1
Attachment #8865183 - Flags: review?(wmccloskey) → review?(aswan)
Attachment #8865184 - Flags: review?(wmccloskey) → review?(aswan)
Attachment #8865185 - Flags: review?(wmccloskey) → review?(aswan)
Comment on attachment 8865183 [details]
Bug 1356546: Part 2 - Use StructuredCloneHolder as transport for MessageManager messages.

https://reviewboard.mozilla.org/r/136852/#review141186

::: toolkit/components/extensions/test/xpcshell/test_ext_runtime_sendMessage_errors.js:21
(Diff revision 4)
>  
>        // Even when the parameters are accepted, we still expect an error
>        // because there is no onMessage listener.
>        [[null, null, null], "Could not establish connection. Receiving end does not exist."],
>  
> -      // Structural cloning doesn't work with DOM but we fall back
> +      // Structured cloning doesn't work with DOM objects

I don't undestand this change, does the old comment describe something that `Cu.cloneInto()` does (did)?
Comment on attachment 8865183 [details]
Bug 1356546: Part 2 - Use StructuredCloneHolder as transport for MessageManager messages.

https://reviewboard.mozilla.org/r/136852/#review141186

> I don't undestand this change, does the old comment describe something that `Cu.cloneInto()` does (did)?

No, the message manager falls back to JSON when structured clone fails, but StructuredCloneHolder is not JSON compatible, so that no longer applies. I have no idea why this was even being tested. That behavior was never intentional or desirable.
Comment on attachment 8865183 [details]
Bug 1356546: Part 2 - Use StructuredCloneHolder as transport for MessageManager messages.

https://reviewboard.mozilla.org/r/136852/#review141268

The changes look good to me except I would expect this to break messages sent to native applicaitons through a port object obtained with `runtime.connectNative()`.  If that's not broken then I'm clearly not actually understanding how the patches here work...
Comment on attachment 8865183 [details]
Bug 1356546: Part 2 - Use StructuredCloneHolder as transport for MessageManager messages.

https://reviewboard.mozilla.org/r/136852/#review141396

::: toolkit/components/extensions/NativeMessaging.jsm:349
(Diff revision 5)
>      }
> +    let msg = holder.deserialize(global);
>      if (Cu.getClassName(msg, true) != "ArrayBuffer") {
>        // This error cannot be triggered by extensions; it indicates an error in
>        // our implementation.
> +      dump(`Hmm... ${Cu.getClassName(msg, true)} ${msg} ${Error().stack}\n`);

whack this before landing
Attachment #8865183 - Flags: review?(aswan) → review+
Comment on attachment 8865184 [details]
Bug 1356546: Part 3 - Use StructuredCloneHolder as transport for proxied message listeners.

https://reviewboard.mozilla.org/r/136854/#review141398
Attachment #8865184 - Flags: review?(aswan) → review+
Comment on attachment 8865185 [details]
Bug 1356546: Part 4 - Use StructuredCloneHolder as transport for proxied method return values.

https://reviewboard.mozilla.org/r/136856/#review141408

I'm not wrapping (pun partially intended) my head around this, can you add some comments explaining it, perhaps to ExtensionUtils.jsm
Attachment #8865185 - Flags: review?(aswan)
Comment on attachment 8865182 [details]
Bug 1356546: Part 1 - Add a StructuredCloneHolder JS helper to hold opaque structured clone blobs.

https://reviewboard.mozilla.org/r/136850/#review141796

::: dom/base/DOMStructuredCloneHolder.h:23
(Diff revision 4)
> +#include "nsWrapperCache.h"
> +
> +namespace mozilla {
> +namespace dom {
> +
> +class DOMStructuredCloneHolder : public nsISupports

This name doesn't seem great. There's nothing about this that really strikes me as DOM-related. Maybe StructuredCloneBlobHolder?

::: dom/base/DOMStructuredCloneHolder.h:25
(Diff revision 4)
> +namespace mozilla {
> +namespace dom {
> +
> +class DOMStructuredCloneHolder : public nsISupports
> +                               , public StructuredCloneHolder
> +                               , public nsWrapperCache

As we discussed, I don't think this class needs to be wrapper cached. It's only held by JS code. That means you should be able to remove the mParent reference and return null from GetParentObject. And you won't need to cycle collect it.

::: dom/base/DOMStructuredCloneHolder.cpp:23
(Diff revision 4)
> +    : StructuredCloneHolder(CloningSupported, TransferringNotSupported,
> +                            StructuredCloneScope::DifferentProcess)
> +    , mParent(aParent)

It seems like this is indented too far.

::: dom/base/DOMStructuredCloneHolder.cpp:118
(Diff revision 4)
> +  }
> +
> +  mBuffer = MakeUnique<JSAutoStructuredCloneBuffer>(mStructuredCloneScope,
> +                                                    &StructuredCloneHolder::sCallbacks,
> +                                                    this);
> +  mBuffer->adopt(Move(data), version, &DOMStructuredCloneHolder::sCallbacks);

Can you use StructureCloneHolder::sCallbacks instead? Otherwise it suggests that we're using our own callbacks.

::: dom/base/DOMStructuredCloneHolder.cpp:126
(Diff revision 4)
> +}
> +
> +bool
> +DOMStructuredCloneHolder::WriteStructuredClone(JSContext* aCx, JSStructuredCloneWriter* aWriter)
> +{
> +  auto& data = mBuffer->data();

I think it would be clear to explicitly declare this as a JSStructuredCloneData (or maybe even a BufferList).

::: dom/base/DOMStructuredCloneHolder.cpp:146
(Diff revision 4)
> +}
> +
> +JSObject*
> +DOMStructuredCloneHolder::WrapObject(JSContext* aCx, JS::HandleObject aGivenProto)
> +{
> +    return StructuredCloneHolderBinding::Wrap(aCx, this, aGivenProto);

Indent is wrong.
Attachment #8865182 - Flags: review?(wmccloskey) → review+
Comment on attachment 8865182 [details]
Bug 1356546: Part 1 - Add a StructuredCloneHolder JS helper to hold opaque structured clone blobs.

https://reviewboard.mozilla.org/r/136850/#review141856

::: dom/bindings/Bindings.conf:808
(Diff revision 5)
>      'headerFile': 'mozilla/dom/WorkerScope.h',
>      'implicitJSContext': [ 'close' ],
>  },
>  
> +'StructuredCloneHolder': {
> +    'headerFile': 'mozilla/dom/DOMStructuredCloneHolder.h',

You shouldn't need the headerFile bit, given the nativeType here.

That said, it's a bit confusing to have a StructuredCloneHolder WebIDL interface that doesn't map to mozilla::dom::StructuredCloneHolder, when the latter actually exists....  I wish we had better naming here.

::: dom/webidl/StructuredCloneHolder.webidl:17
(Diff revision 5)
> +interface StructuredCloneHolder {
> +  /**
> +   * Serializes the given value to an opaque structured clone blob, and
> +   * returns the result.
> +   *
> +   * Note: The value is unwrapped and serialized in the scope of its own

This seems pretty dangerous to me from a security point of view.  Why are we doing that?  Just for performance reasons?

Is the intended use to deserialize into a global that is same-origin with the global we serialized in?

::: dom/webidl/StructuredCloneHolder.webidl:22
(Diff revision 5)
> +   * Note: The value is unwrapped and serialized in the scope of its own
> +   * global, so the semantics of X-ray security wrappers do not come into play
> +   * during the serialization process.
> +   */
> +  [Throws]
> +  static StructuredCloneHolder serialize(any data);

Is there a reason this is a static method and not just a constructor?
Comment on attachment 8865182 [details]
Bug 1356546: Part 1 - Add a StructuredCloneHolder JS helper to hold opaque structured clone blobs.

https://reviewboard.mozilla.org/r/136850/#review141856

> You shouldn't need the headerFile bit, given the nativeType here.
> 
> That said, it's a bit confusing to have a StructuredCloneHolder WebIDL interface that doesn't map to mozilla::dom::StructuredCloneHolder, when the latter actually exists....  I wish we had better naming here.

Yeah, the problem is that I want to call the JS class StructuredCloneHolder, but it didn't seem like a good idea to try to extend that object to handle the binding version as well.

> This seems pretty dangerous to me from a security point of view.  Why are we doing that?  Just for performance reasons?
> 
> Is the intended use to deserialize into a global that is same-origin with the global we serialized in?

It generally winds up deserializing to the same global, yes. The only current exception is when the origin compartment is chrome-privileged, and the target context is an unprivileged context on the other side of a message manager (where this takes the place of a cloneInto).

But it's not especially dangerous. Not any more dangerous than X-rays, anyway. The actual serialization happens with the privileges of the source compartment, which means the behavior is basically equivalent to that compartment doing sendMessage, or something along those lines. If we tried to serialize something we'd waived X-rays on, on the other hand, that would be incredibly dangerous.

And, yes, we're mainly doing it for performance reasons. X-ray overhead is apparently the main reason these operations are so slow, followed by the repeated clones that happen when we send the data over message managers.

> Is there a reason this is a static method and not just a constructor?

Probably not. I'm not even sure why I chose to make it a static method at this point.
> The only current exception is when the origin compartment is chrome-privileged, and the target context is an unprivileged context on the other side of a message manager

I just want to make sure we agree on the terminology.

Is "origin compartment" the compartment of the original "any" that we serialize?  Or is it the compartment where we do the serialize() call?  The latter is obviously always chrome-privileged, because it's calling a ChromeOnly API.  But if the compartment of that "any" is chrome-privileged, we won't have Xrays to it anyway, right?

> But it's not especially dangerous

I agree that the specific cases of "the 'any' came from a chrome compartment anyway and is being posted to a non-chrome compartment" and "we are posting to a compartment that is same-origin with the place the 'any' came from" are not dangerous.  Are those cases the only ones that can arise?  That is, can we fail deserialization if we're deserializing into a more privileged global than the global the original thing came from?

> The actual serialization happens with the privileges of the source compartment

Sure.

> which means the behavior is basically equivalent to that compartment doing sendMessage, or something along those lines.

Right, but what compartments would be Xray _targets_ and be able to sendMessage?

> If we tried to serialize something we'd waived X-rays on

Uh... once you waive Xrays, you're obviously not going to be safe.  The whole point of Xrays being able to protect you is that you _don't_ waive them.

> X-ray overhead is apparently the main reason these operations are so slow

I would really like to understand the cases in which the argument to serialize() is an Xray and what the expected deserialization global looks like in those cases.
(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #40)
> > The only current exception is when the origin compartment is chrome-privileged, and the target context is an unprivileged context on the other side of a message manager
>
> I just want to make sure we agree on the terminology.
>
> Is "origin compartment" the compartment of the original "any" that we
> serialize?

Yes.

> I agree that the specific cases of "the 'any' came from a chrome compartment
> anyway and is being posted to a non-chrome compartment" and "we are posting
> to a compartment that is same-origin with the place the 'any' came from" are
> not dangerous.  Are those cases the only ones that can arise?

At the moment, yes, the only time that we serialize an object from an
unprivileged compartment, we deserialize it in a similarly privileged
compartment that it's sending sending a message to.

> That is, can we fail deserialization if we're deserializing into a more
> privileged global than the global the original thing came from?

We probably could, but I don't think that's necessary. See below.

> > which means the behavior is basically equivalent to that compartment doing sendMessage, or something along those lines.
>
> Right, but what compartments would be Xray _targets_ and be able to
> sendMessage?

Sorry, I meant postMessage, particularly window.postMessage, which is one of
the few methods we promote for communication between privileged and
unprivileged scopes, along with X-rays. When an unprivileged scope posts a
message to a privileged window, the message is structured clone serialized
from the scope sending the message and deserialized in the target scope,
without any X-rays involved on either end.

The behavior here is equivalent to the behavior of postMessage API here, and
for the most part used to implement a similar messaging API.

The deserialized result provides the same protections that X-rays do, because
all of the result objects are plain objects (or nearly plain objects, like
ImageData), with no getters or unpredictable properties, and none are
cross-compartment wrappers.

> > If we tried to serialize something we'd waived X-rays on
>
> Uh... once you waive Xrays, you're obviously not going to be safe.  The
> whole point of Xrays being able to protect you is that you _don't_ waive
> them.

To be clear, I'm talking about the difference between something like
`JSON.stringify(Cu.waiveXrays(obj))` from a privileged scope vs.
`JSON.stringify(obj)` from the unprivileged scope that the object comes from.
The resulting string is more or less safe either way, but in the former case,
we're not protected against accessing objects that the source compartment has
references to but not privileges to access.

> > X-ray overhead is apparently the main reason these operations are so slow
>
> I would really like to understand the cases in which the argument to
> serialize() is an Xray and what the expected deserialization global looks
> like in those cases.

The only cases where that currently happens is when a message is sent via one
of the WebExtension messaging APIs. E.g.,

    browser.tabs.sendMessage(tabId, message);

where the message is an arbitrary value belonging to the message scope, and
the message is eventually deserialized into a content script sandbox belonging
to the add-on. Or, similarly,

    browser.runtime.sendMessage(message);

where the message is sent to all of the privileged pages belonging to the
add-on, and likewise deserialized there.
> When an unprivileged scope posts a message to a privileged window

Then the privileged window is not trusting the resulting thing in various ways, I hope.  I mean, you can trust it to not have any accessors, and you presumably don't expect it to have any DOM objects, so a lot of the concerns Xrays are meant to address don't apply.  But you still have to be careful about what you do with the data; e.g. you shouldn't eval it.  ;)

> The deserialized result provides the same protections that X-rays do

The thing I'm worried about is a confused-deputy attack where we have privileged code A sending a message to privileged code B but producing the message by serializing an unprivileged object from C.  If A does the serialization over Xrays, C is pretty limited in what it can do to mess up the message.  If not, it's equivalent to C sending a message to B, which B might not be expecting.  If it were a direct postMessage, B could check the origin of the sender, but that's not possible with the setup we're discussing here.

There's no problem if the confused deputy has no more permissions than yourself, of course; that is, if B can't do anything C couldn't do anyway, then we don't care if C can trick B into doing things, since they're all things C could have done itself.  That's why I was asking whether we could enforce such a restriction.

> where the message is an arbitrary value belonging to the message scope, and
> the message is eventually deserialized into a content script sandbox belonging
> to the add-on.

I'm afraid I don't know enough about our webextension setup.  :( What are the principals of the message scope and the sandbox involved?

> where the message is sent to all of the privileged pages belonging to the
> add-on, and likewise deserialized there

Again, not sure what the principals involved are...
(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #42)
> > When an unprivileged scope posts a message to a privileged window
>
> Then the privileged window is not trusting the resulting thing in various
> ways, I hope.  I mean, you can trust it to not have any accessors, and you
> presumably don't expect it to have any DOM objects, so a lot of the concerns
> Xrays are meant to address don't apply.  But you still have to be careful
> about what you do with the data; e.g. you shouldn't eval it.  ;)

Agreed. But X-rays don't give us any more protections than a structured clone,
here.

> > The deserialized result provides the same protections that X-rays do
>
> The thing I'm worried about is a confused-deputy attack where we have
> privileged code A sending a message to privileged code B but producing the
> message by serializing an unprivileged object from C.  If A does the
> serialization over Xrays, C is pretty limited in what it can do to mess up
> the message.  If not, it's equivalent to C sending a message to B, which B
> might not be expecting.  If it were a direct postMessage, B could check the
> origin of the sender, but that's not possible with the setup we're
> discussing here.
>
> There's no problem if the confused deputy has no more permissions than
> yourself, of course; that is, if B can't do anything C couldn't do anyway,
> then we don't care if C can trick B into doing things, since they're all
> things C could have done itself.  That's why I was asking whether we could
> enforce such a restriction.

Currently, we only ever use this holder object with messages sent across a
message manager, so we have the same level of protection against this kind of
attack with it as we would without it (since any structured clone blob that
could be created without X-rays could also be created with them).

I suppose we could serialize the source principal, and do the check at the
other end, which would give us a bit more protection than we had before, but
I'm still not sure it's worth the effort.

> > where the message is an arbitrary value belonging to the message scope, and
> > the message is eventually deserialized into a content script sandbox belonging
> > to the add-on.
> 
> I'm afraid I don't know enough about our webextension setup.  :( What are
> the principals of the message scope and the sandbox involved?

In the case where we're sending a message to a content script, the source
compartment is a codebase principal from a moz-extension: page, and the
sandbox is an expanded principal with the same codebase principal as the
extension page, and a codebase principal for the content page the sandbox is
tied to.

> > where the message is sent to all of the privileged pages belonging to the
> > add-on, and likewise deserialized there
>
> Again, not sure what the principals involved are...

On both sides, codebase principals for moz-extension: URLs. Usually the same
principal on both ends, but it's possible to send a message from one extension
to another (in which case it comes through on a different channel, onMessage
vs. onMessageExternal).
> But X-rays don't give us any more protections than a structured clone, here.

Not for the _receiver_.  But the _sender_ is harder to fool if it's using Xrays: if it sends an object it doesn't control, the thing that does control it has a harder time sneaking things into the message.

> so we have the same level of protection against this kind of
> attack with it as we would without it

Again, it really depends on where the message value comes from and who's doing the sending.

> but it's possible to send a message from one extension to another 

OK, it sounds like in that case the principals would not subsume each other, right?  So putting in the check I'm suggesting would make this case fail?
(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #44)
> > But X-rays don't give us any more protections than a structured clone, here.
>
> Not for the _receiver_.  But the _sender_ is harder to fool if it's using
> Xrays: if it sends an object it doesn't control, the thing that does control
> it has a harder time sneaking things into the message.

I think it depends on who you mean by sender.

If you mean the chrome code that's calling serialize, it doesn't make much
difference, since the content compartment that it's serializing from still has
nearly complete control over the result.

If you mean the content side that's calling (e.g.) sendMessage, it still
doesn't make much difference, since any objects that it has access to are
either same origin or X-ray protected. Unless they've waived X-rays, in which
case any security issues are of their own making.

> > so we have the same level of protection against this kind of
> > attack with it as we would without it
>
> Again, it really depends on where the message value comes from and who's
> doing the sending.

Well, it really doesn't, because we're doing a structured clone either way, so
the receiving side doesn't get any more information about the origin of the
cloned data than they would with a StructuredCloneHolder object.

> > but it's possible to send a message from one extension to another
>
> OK, it sounds like in that case the principals would not subsume each other,
> right?  So putting in the check I'm suggesting would make this case fail?

If we enforced strict origin semantics, yes. If we simply prevented
deserializing into a chrome privileged compartment, no.
> If you mean the chrome code that's calling serialize

Yes, that is who I mean.

> since the content compartment that it's serializing from still has
> nearly complete control over the result

Even if it serializes unwaived xrays?  I don't see how.  Now it might be that the problem in that case would be that we'd never be able to send anything useful at all, right?

> it still doesn't make much difference, since any objects that it has
> access to are either same origin or X-ray protected.

OK, so I feel like I'm missing something here.  How would the caller of sendMessage() end up with an Xray?  Is this a case where that caller is a content script and the thing it passes to sendMessage a web page object?

In any case, I'm not sure what "X-ray protected" means here and why you say it helps.  If the caller of sendMessage passes in a value that is an Xray, does that value just get handed to chrome code that calls serialize?  Because if so, then the thing that gets serialized will be the target of the original Xray, not the Xray itself, right?

> because we're doing a structured clone either way

Yes, but a structured clone of an Xray and a structured clone of its target may well look quite different!
(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #46)
> > since the content compartment that it's serializing from still has
> > nearly complete control over the result
>
> Even if it serializes unwaived xrays?  I don't see how.  Now it might be
> that the problem in that case would be that we'd never be able to send
> anything useful at all, right?

Structured clone only allows creating simple objects in a single compartment.
Any structured clone object graph that can be created without X-rays can be
created with them. Or, at least, I can't think of any exceptions.

> > it still doesn't make much difference, since any objects that it has
> > access to are either same origin or X-ray protected.
>
> OK, so I feel like I'm missing something here.  How would the caller of
> sendMessage() end up with an Xray?  Is this a case where that caller is a
> content script and the thing it passes to sendMessage a web page object?

Yes, I guess I forgot to mention that case. A content script can send messages
other pages (but not directly to other content scripts) within the same
extension.

> In any case, I'm not sure what "X-ray protected" means here and why you say
> it helps.  If the caller of sendMessage passes in a value that is an Xray,
> does that value just get handed to chrome code that calls serialize?

No, the serialization happens in the compartment of the code that called
sendMessage. In this case, the protection would be for that
(non-chrome-privileged) caller.

> Because if so, then the thing that gets serialized will be the target of the
> original Xray, not the Xray itself, right?

Any X-rays in the compartment of the object we're serializing stay X-rays.
Unwrapping only happens for the original object being passed to serialize,
then everything else happens as normal in that object's compartment.

> > because we're doing a structured clone either way
>
> Yes, but a structured clone of an Xray and a structured clone of its target
> may well look quite different!

They may, but either way, if the (non-chrome-privileged) compartment that
we're serializing the message from knows that the serialization is happening
via an X-ray, it has complete control over the results.
> Any structured clone object graph that can be created without X-rays can be
> created with them.

Hmm.  Because Xrays to vanilla JS Object will let you see value properties, I guess?

I guess the only worry is then cases in which some sort of sanity-checking happens via an Xray but the serialization happens on a non-Xray and produces something different than what was sanity-checked...

> No, the serialization happens in the compartment of the code that called
> sendMessage.

I don't see how.  It's not the sendMessage call that serializes, right?

So say the content script has an Xray to a web page object.  It calls sendMessage with that object.  This ends up calling some chrome JS, presumably, which then calls the serialize() method we're adding in this bug, right?

As soon as we've landed in the chrome JS, now the chrome JS has an Xray to the web page object.  If it calls serialize(), we will enter the _web_page_ compartment (because CheckedUnwrap returns the web page object) and serialize in there, right?
(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #48)
> > Any structured clone object graph that can be created without X-rays can be
> > created with them.
>
> Hmm.  Because Xrays to vanilla JS Object will let you see value properties,
> I guess?

Yes.

> I guess the only worry is then cases in which some sort of sanity-checking
> happens via an Xray but the serialization happens on a non-Xray and produces
> something different than what was sanity-checked...

That could happen, yes. We're not using this interface in any way where that
would matter, but someone else might, I suppose. So I'll add some paragraphs
to the API docs to explain the possible risks.

> > No, the serialization happens in the compartment of the code that called
> > sendMessage.
>
> I don't see how.  It's not the sendMessage call that serializes, right?
>
> So say the content script has an Xray to a web page object.  It calls
> sendMessage with that object.  This ends up calling some chrome JS,
> presumably, which then calls the serialize() method we're adding in this
> bug, right?

It's not possible to pass a cross-compartment wrapper directly to a function
exported via exportFunction or cloneInto (as all of those API methods are)
without passing a special flag (which we don't), and it's not possible to
access a cross-compartment object via an X-ray without waiving (which we also
don't), so this isn't a concern. Or, rather, it's a concern, but it's a
concern that we're intentionally mitigating.
I'm not following that last paragraph at all.  At some point we have content script calling a system-principal function, right?  It passes it some object.  What prevents that function call if the object happens to be an Xray?
The function that the content script or extension script calls was exported to its compartment via Cu.exportFunction. The wrappers created by that helper block calls with cross-origin arguments unless the allowCrossOriginArguments option is passed:

https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Language_Bindings/Components.utils.exportFunction
> The wrappers created by that helper block calls with cross-origin arguments unless the
> allowCrossOriginArguments option is passed

Looking at the implementation of allowCrossOriginArguments, I'm this does not look like a correct description of the behavior.  If that boolean is not set, then arguments that are wrappers are allowed only if the wrapper principal subsumes the wrapped-object principal.  Which in the case of Xrays it does, usually (the exception is web-exposed cross-origin Xrays).

That behavior matches the documentation at https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Language_Bindings/Components.utils.exportFunction#Cross-origin_checking which says that the check is that the caller subsumes the unwrapped argument.  The intent is to prevent escalation of privileges past what the caller could have managed to do itself.  It does nothing to help the callee if the callee does unsafe things that the caller could have done too.
OK, that's a fair point. In that case it might be worth passing a separate scope argument to wrap for.
Comment on attachment 8865185 [details]
Bug 1356546: Part 4 - Use StructuredCloneHolder as transport for proxied method return values.

https://reviewboard.mozilla.org/r/136856/#review145744

I think that if anybody other than :kmag coming along and wants to touch this code, the odds that they will understand the nuances in this patch are close to zero.  I think it would be much more readable if NoCloneSpreadArgs didn't also act as an array but we just used its unwrappedValues property consistently.  Kris expressed an unwillingness to do this on IRC but the reasons were vague (xray wrappers of arrays are flaky).  If that's a serious concern, we have several APIs that receive arrays that are vulnerable to this problem.  Or, given that NoCloneSpreadArgs in this patch was just deserialized from a StructuredCloneHolder, can we just unwrap the target array to check its length?
Attachment #8865185 - Flags: review?(aswan)
Comment on attachment 8865185 [details]
Bug 1356546: Part 4 - Use StructuredCloneHolder as transport for proxied method return values.

https://reviewboard.mozilla.org/r/136856/#review148458
Attachment #8865185 - Flags: review?(aswan) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/f18f7ef1d81f273f6d0b9b4afd1e0980b65ae523
Bug 1356546: Part 1 - Add a StructuredCloneHolder JS helper to hold opaque structured clone blobs. r=billm

https://hg.mozilla.org/integration/mozilla-inbound/rev/850918e6790b80ded3d5b70d4fdb845b6cb24408
Bug 1356546: Part 2 - Use StructuredCloneHolder as transport for MessageManager messages. r=aswan

https://hg.mozilla.org/integration/mozilla-inbound/rev/fe952a030435f0690bd4f64d6299201b9734cb47
Bug 1356546: Part 3 - Use StructuredCloneHolder as transport for proxied message listeners. r=aswan

https://hg.mozilla.org/integration/mozilla-inbound/rev/c3bf2490530f5ac768040043b8d0dd55ce6c5a03
Bug 1356546: Part 4 - Use StructuredCloneHolder as transport for proxied method return values. r=aswan
Depends on: 1370884
Depends on: 1371278
Depends on: 1373579
Product: Toolkit → WebExtensions
Performance Impact: --- → P1
Whiteboard: [qf:p1][triaged] → [triaged]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: